Try pnpm (take two) #38624
Try pnpm (take two) #38624
Conversation
sarayourfriend
force-pushed
the
try/pnpm-fresh
branch
2 times, most recently
from
9de3efb
to
96edaf8
Compare
|
The |
|
Failing on installation due to some patches not applying correctly. Either we need to hoist the packages we're patching or I think we could also just change the patch diffs to point to the |
|
I think I need to bust the cache somehow 🤔 Seems like the cache calculation should include |
|
That worked! Now there seem to be some TypeScript resolution issues. Cursory searching seems to indicate TypeScript should "just work" with pnpm but clearly there's some issue with our setup here. I also can't reproduce the issue locally yet, so that's probably the first step. Will report back anything I find 🙂 |
|
Aha! Looking at the first couple types that aren't being resolved it seems like they're indeclared development dependencies! |
sarayourfriend
force-pushed
the
try/pnpm-fresh
branch
2 times, most recently
from
9858629
to
ee56705
Compare
|
Had to squash changes to make rebasing easier when |
|
The plugin-zip building script needs to be updated to use |
|
This will on some level be blocked by until this PR is merged for the compressed-size-check action: preactjs/compressed-size-action#68 |
|
Using |
| with: | ||
| node-version: ${{ matrix.node }} | ||
| cache: npm | ||
| node-version: ${{ matrix-node }} |
There was a problem hiding this comment.
I think this should still be matrix.node?
There was a problem hiding this comment.
Whoops, yes it should be!
| with: | ||
| node-version: 14 |
There was a problem hiding this comment.
The default is set to 14. I only kept passing it to the action in places that were still using the matrix strategy for it.
| - name: Install | ||
| if: inputs.install | ||
| shell: bash | ||
| run: "pnpm install ${{ inputs.frozen-lockfile && '--frozen-lockfile' || '' }}" |
There was a problem hiding this comment.
Looking at the doc, seems like we don't have to pass --frozen-lockfile to true in CI since it's the default.
There was a problem hiding this comment.
Interesting. I don't really understand the "For CI: true" remark. How can it tell if it's in a CI environment? This isn't described anywhere in the documentation as far as I could tell... might have to dig into the code to find that out when I come back to this.
I do think you're right in questioning why we would need it to ever not install with a frozen-lockfile on CI though. Maybe this PR is the right time to investigate that? 🤔
There was a problem hiding this comment.
I believe most programs depend on the env variable process.env.CI to exist in CI environment. Most CI platforms, including GH Actions, set this variable.
| required: false | ||
| description: Optional working directory to run installation in. | ||
| frozen-lockfile: | ||
| default: false |
There was a problem hiding this comment.
Why do we want to default it to false?
There was a problem hiding this comment.
I'm not sure! There were places that were doing just npm install rather than npm ci and I didn't want to change their behavior in this PR.
There was a problem hiding this comment.
In bd1d499 I changed it to just do a regular frozen-lockfile install everywhere relying on the default behavior in CI you mentioned. I can't think of a reason why CI should need to use a non-frozen install anywhere anyway.
| default: false | ||
| description: Whether to install dependencies using `--frozen-lockfile`. | ||
| node-version: | ||
| default: 14 |
There was a problem hiding this comment.
I think this is nice that we don't have to repeat passing node-version in every workflow if it's just running the default version. We can just update this file when upgrading node to a major version.
There was a problem hiding this comment.
I guess this could also be changed to node-version-file: '.nvmrc' so that https://github.com/WordPress/gutenberg/blob/trunk/.nvmrc is the only place to change the version.
https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#node-version-file
There was a problem hiding this comment.
Nice find! That sounds like a good option 🎉 We'd need to keep around node-version still for the workflows that use a node version matrix but referring to the .nvmrc file for the default is 💯
There was a problem hiding this comment.
.nvmrc plays an important role for branches where we apply fixes to older WordPress major releases like wp/5.1.
There was a problem hiding this comment.
Okay in bd1d499 I tried to set it up to just use .nvmrc by default unless a specific node-version is passed in (to preserve the ability for workflows to use the node version matrix, if that's still necessary). Hopefully it works, I'm not too strong on GitHub action expressions but should only need a little tweaking if it doesn't work right off the bat.
gziolo
added
[Type] Build Tooling
…t versions declared
sarayourfriend
force-pushed
the
try/pnpm-fresh
branch
from
15677ce
to
618ae4f
Compare
|
I just realized I'd been fighting dependency resolution issues through hoisting that are meant to be resolved through cross-workspace-package dependencies... Turns out |
See https://github.com/microsoft/rushstack/tree/master/eslint/eslint-patch README for explanation why this is needed in a monorepo setting.
|
It turns out the patch will only work for a top-level eslint configuration. The nested In general I think it'd be a good idea to continue documenting hoists. I feel like we can avoid just hoisting everything (which has it's own set of problems anyway) and everything that is documented and hoisted should theoretically be able to eventually be removed with the right dependency declarations. Of course, that's with the exception of stylelint, eslint and anything used by expo, because those tools all have their own non-modern module resolution strategies that cause problems with Consumers of the Anyway, it's kind of a mess but unavoidable at this point as far as I can tell. |
|
Regarding the current state of the unit tests: I think they might be flagging a potentially problematic (not generally, just for The fact that the e2e tests are passing consistently is exciting though! |
|
Closing this draft. If anyone wants to pick this up once React Native is supported by pnpm then you'll have some starting points here though I would be things have changed in the mean time. The new npm versions might not make this change worthwhile anyway. |
Description
I'll force push this onto
try/pnpmin #37324 once this is working with CI so we can preserve the original PR discussion.Testing Instructions
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).